Skip to content

Conversation

@luciechoi
Copy link
Collaborator

@luciechoi luciechoi commented Jan 7, 2026

Part of #7979

  • Add SampledTexture2D resource type in the vk namespace.
  • Add .Sample(float2 location) method.
  • Add default type parameter float4.
  • Allow optional arguments int2 offset, float clamp, uint status.
  • Add .CalculateLevelOfDetail(float2 location) method.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2026

✅ With the latest revision this PR passed the C/C++ code formatter.

@luciechoi luciechoi marked this pull request as draft January 7, 2026 09:03
Copy link
Collaborator

@s-perron s-perron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a reasonable direction. There are some issues that we need to figure out how to handle.

// CHECK: [[in_var_TEXCOORD:%[a-zA-Z0-9_]+]] = OpVariable %_ptr_Input_v2float Input
// CHECK: [[out_var_SV_Target:%[a-zA-Z0-9_]+]] = OpVariable %_ptr_Output_v4float Output

vk::SampledTexture2D<float4> tex : register(t0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you test this with an int so it is different then the next test? That we will know if you accidentally always output a float type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing out, added a test for vk::SampledTexture2D<uint> type.

Comment on lines 4021 to 4024
QualType float4Type =
LookupVectorType(HLSLScalarType::HLSLScalarType_float, 4);
recordDecl = DeclareVkSampledTexture2DType(*m_context, m_vkNSDecl,
float2Type, float4Type);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the result of the sample have to be a 4 element vector? What happens if the template type is a scalar?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, if a template type is scalar, the results are extracted. Added a test.

Comment on lines 625 to 628
if (isSampledTexture(imageType)) {
sampledImage = image;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we assert that the sampler is false? Not necessary, but this function has a complicated interface, and it could be easy to use it incorrectly.

Is there documentation for this function to update?

Copy link
Collaborator Author

@luciechoi luciechoi Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we assert that the sampler is false?

Added.

Is there documentation for this function to update?

Updated the function documentation.

@luciechoi luciechoi force-pushed the sampledtexture2d branch 3 times, most recently from 67fef39 to 8030b4e Compare January 8, 2026 04:56
@luciechoi luciechoi marked this pull request as ready for review January 8, 2026 05:03
@luciechoi
Copy link
Collaborator Author

Thanks for the early review! I've updated the method to take in optional arguments.

@luciechoi luciechoi requested a review from s-perron January 8, 2026 05:06
@luciechoi luciechoi changed the title [SPIR-V] Add vk::SampledTexture2D resource type and .Sample() method. [SPIR-V] Add vk::SampledTexture2D resource type and .Sample(), CalculateLevelOfDetail() method. Jan 9, 2026
@luciechoi luciechoi changed the title [SPIR-V] Add vk::SampledTexture2D resource type and .Sample(), CalculateLevelOfDetail() method. [SPIR-V] Add vk::SampledTexture2D resource type and .Sample(), .CalculateLevelOfDetail() method. Jan 9, 2026
@damyanp
Copy link
Member

damyanp commented Jan 12, 2026

If this is worth mentioning in the release notes, please add something to https://github.com/microsoft/DirectXShaderCompiler/blob/main/docs/ReleaseNotes.md as appropriate.

Copy link
Collaborator

@s-perron s-perron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good to me. A few minor code style issues to fix up.

Comment on lines +289 to +291
/// If imageType is not a sampled image type, the OpSampledImage* instructions
/// will be generated.
///
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is confusing. Can you rephrase?

Suggested change
/// If imageType is not a sampled image type, the OpSampledImage* instructions
/// will be generated.
///
/// If the of `image` is a sampled image, then that image will be sampled. In this case, `sampler` must be `nullptr`. If `image` is not a sampled image, a sampled image will be create by combining `image` and `sampler`.
///

You'll need to fix the formatting.

Comment on lines +1402 to +1429
// Sample(location, offset)
QualType params2[] = {float2Type, int2Type};
StringRef names2[] = {"location", "offset"};
CXXMethodDecl *sampleDecl2 = CreateObjectFunctionDeclarationWithParams(
context, recordDecl, paramType, params2, names2,
context.DeclarationNames.getIdentifier(&context.Idents.get("Sample")),
/*isConst*/ true);
sampleDecl2->addAttr(HLSLIntrinsicAttr::CreateImplicit(
context, "op", "", static_cast<int>(hlsl::IntrinsicOp::MOP_Sample)));
sampleDecl2->addAttr(HLSLCXXOverloadAttr::CreateImplicit(context));

// Sample(location, offset, clamp)
QualType params3[] = {float2Type, int2Type, floatType};
StringRef names3[] = {"location", "offset", "clamp"};
CXXMethodDecl *sampleDecl3 = CreateObjectFunctionDeclarationWithParams(
context, recordDecl, paramType, params3, names3,
context.DeclarationNames.getIdentifier(&context.Idents.get("Sample")),
/*isConst*/ true);
sampleDecl3->addAttr(HLSLIntrinsicAttr::CreateImplicit(
context, "op", "", static_cast<int>(hlsl::IntrinsicOp::MOP_Sample)));
sampleDecl3->addAttr(HLSLCXXOverloadAttr::CreateImplicit(context));

// Sample(location, offset, clamp, status)
QualType params4[] = {float2Type, int2Type, floatType,
context.getLValueReferenceType(uintType)};
StringRef names4[] = {"location", "offset", "clamp", "status"};
CXXMethodDecl *sampleDecl4 = CreateObjectFunctionDeclarationWithParams(
context, recordDecl, paramType, params4, names4,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function will be come very big. Could you break it up into smaller functions? This block could be its own. Call it addSampeFunction. You can group other functions in the same way.

Comment on lines +4016 to +4026
} else if (kind == AR_OBJECT_VK_SAMPLED_TEXTURE2D) {
if (!m_vkNSDecl)
continue;
recordDecl = DeclareVkSampledTexture2DType(
*m_context, m_vkNSDecl,
LookupVectorType(HLSLScalarType::HLSLScalarType_float, 2),
LookupVectorType(HLSLScalarType::HLSLScalarType_int, 2),
LookupVectorType(HLSLScalarType::HLSLScalarType_float, 4));
recordDecl->setImplicit(true);
m_vkSampledTexture2DTemplateDecl =
recordDecl->getDescribedClassTemplate();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add some tests in sema that look for errors. What if an invalid type is given as a template parameter?

Comment on lines +1374 to +1376
QualType float2Type,
QualType int2Type,
QualType float4Type) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you planning on make this function more general? For example could float2Type become CoordinateType? Then for Textur3D it is just a float?

If not, why pass these in as parameters? Could we simplify the interface an build them in this function?

auto loweredType = lowerType(getElementType(astContext, sampledType), rule,
/*isRowMajor*/ llvm::None, srcLoc);

// Treat bool textures as uint for compatibility with OpTypeImage.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not about compatibility with OpTypeImage. Bool does not have a defined size in SPIR-V, so it cannot be used in the external interface.

Comment on lines +4462 to +4469
auto *objectInfo = loadIfGLValue(imageExpr);
auto *samplerState =
isSampledTexture(imageType) ? nullptr : doExpr(expr->getArg(0));
auto *coordinate = isSampledTexture(imageType) ? doExpr(expr->getArg(0))
: doExpr(expr->getArg(1));

auto *sampledImage =
isSampledTexture(imageType)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are checking isSampledTextured a lot in this code. You should just use a single if-statement.

Comment on lines +5848 to +5856
if (numArgs > 1) {
handleOffsetInMethodCall(expr, 1, &constOffset, &varOffset);
}
if (numArgs > 2) {
clamp = doExpr(expr->getArg(2));
}
if (numArgs > 3) {
status = doExpr(expr->getArg(3));
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You eventually going to have to add all of the checks below when you add all of the appropriate overload. We should try to avoid adding duplicating the argument parsing here.

You could try doing:

uint offset = 0;
auto *image = loadIfGLValue(imageExpr);
auto *sampler = nullptr;

if (!isSampledTexture(imageType)) {
  sampler = doExpr(expr->getArg(0));
  offset = 1;
}

Then all of the later accesses become to expr->getArg(offset + ); For example:

auto *coordinate = doExpr(expr->getArg(offset));

This will avoid repeating the logic.

@s-perron s-perron requested a review from llvm-beanz January 18, 2026 01:11
@s-perron
Copy link
Collaborator

If this is worth mentioning in the release notes, please add something to https://github.com/microsoft/DirectXShaderCompiler/blob/main/docs/ReleaseNotes.md as appropriate.

It will not be fully completed for this release. We should mention it in the next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

3 participants